[Woo POS] Cannot reconnect to Reader after turning internet off and on #12569

AnirudhBhat · 2024-09-10T04:11:10Z

Description

This PR addresses the issue of handling no internet connectivity when attempting to connect to the card reader.

Previous Behavior:
When attempting to connect the card reader without an internet connection, the app would not provide any feedback or response. This was due to the app first checking whether all onboarding conditions were met before initiating the card reader connection. In the case of no internet, the app would navigate to an onboarding screen that explains the need for an internet connection. However, since the onboarding screens were not integrated into the POS mode, the app became unresponsive.

Updated Behavior:
With this PR, we now check for internet connectivity before attempting to start the WooPosCardReaderActivity. If no internet connection is detected, the activity is not launched, and instead, a toast message is displayed asking the user to ensure a proper internet connection before proceeding.

Steps to reproduce

Navigate to POS mode (More menu -> POS mode)
Click on connect to card reader button
While connecting, turn off the internet
Cancel the ongoing connection dialog
Try to connect to card reader again and notice the button won't respond anymore.

Testing information

I've tested this on tablet emulator by turning on and off internet.

Images/gif

Before

Card.Reader.Disconnected.When.No.Internet.Connection.mov

After

card_reader_internet_issue.mp4

I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

…ct to card reader.

…ernet while trying to connect card reader.

…re is no internet.

dangermattic · 2024-09-10T04:13:28Z

	2 Warnings
⚠️	Class `Toast` is missing tests, but `unit-tests-exemption` label was set to ignore this.
⚠️	This PR is assigned to the milestone 20.4. The due date for this milestone has already passed. Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

wpmobilebot · 2024-09-10T04:22:54Z

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`65f8eeb`
	Direct Download	`woocommerce-wear-prototype-build-pr12569-65f8eeb.apk`

wpmobilebot · 2024-09-10T04:34:33Z

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`65f8eeb`
	Direct Download	`woocommerce-prototype-build-pr12569-65f8eeb.apk`

codecov-commenter · 2024-09-10T04:43:07Z

Codecov Report

Attention: Patch coverage is 47.61905% with 11 lines in your changes missing coverage. Please review.

Project coverage is 40.61%. Comparing base (af19b54) to head (65f8eeb).
Report is 17 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...erce/android/ui/woopos/home/WooPosHomeViewModel.kt	33.33%	6 Missing ⚠️
...erce/android/ui/woopos/util/WooPosNetworkStatus.kt	0.00%	4 Missing ⚠️
...d/ui/woopos/home/toolbar/WooPosToolbarViewModel.kt	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##              trunk   #12569   +/-   ##
=========================================
  Coverage     40.61%   40.61%           
- Complexity     5680     5681    +1     
=========================================
  Files          1228     1229    +1     
  Lines         69103    69122   +19     
  Branches       9572     9574    +2     
=========================================
+ Hits          28064    28073    +9     
- Misses        38456    38466   +10     
  Partials       2583     2583

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

samiuelson

Thanks for the PR, @AnirudhBhat! I added a few improvement ideas. Let me know what you think.

samiuelson · 2024-09-12T15:16:16Z

WooCommerce/src/main/res/values/strings.xml

@@ -4251,6 +4251,7 @@
    <string name="woopos_floating_toolbar_menu_open_content_description">Toolbar with card reader status. Menu is open. Double tap to interact.</string>
    <string name="woopos_floating_toolbar_pop_up_menu_content_description">Open toolbar menu</string>
    <string name="woopos_floating_toolbar_pop_up_menu_open_content_description">Popup menu with options. Swipe to navigate through items.</string>
+    <string name="woopos_no_internet_message">It looks like you\'re not connected to the internet.\n\nEnsure your Wi-Fi is turned on. If you\'re using mobile data, make sure it\'s enabled in your device settings.</string>


The string is not rendered correctly, new lines are cut:

Thanks for the heads up! Fixed

samiuelson · 2024-09-12T15:29:58Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/WooPosHomeViewModel.kt

        )
    )
    val state: StateFlow<WooPosHomeState> = _state

+    private val _toastEvent = MutableSharedFlow<Int>()


💡 I'd improve the readability of the _toastEvent prop type. I'd personally wrap the Int with a dedicated data class, e.g.

data class Toast( @StringRes val message: Int, )

even better could be to emit ready to use string:

data class Toast( val message: String, )

or alternatively

import kotlin.Int as ToastMessageStringRes private val _toastEvent = MutableSharedFlow<ToastMessageStringRes>() val toastEvent: SharedFlow<ToastMessageStringRes> = _toastEvent

Thanks for the suggestion! Improved it here: f49db88

samiuelson

LGTM

AnirudhBhat added 8 commits September 9, 2024 12:27

Add class to detect network availability

9fb68e0

Add NoInternet event to communicate between children and parent

10695d7

Trigger NoInternet event if there's not internet when trying to conne…

b98c2df

…ct to card reader.

Trigger shared flow event when there is no internet from home view model

41de09a

Use LaunchedEffect to display toast to user when there is no internet.

1842108

Fix failing test class

a997fdc

Add test to verify that proper event is triggered when there's no int…

63dc317

…ernet while trying to connect card reader.

Add test to verify that connect to card reader is not called when the…

3d6d617

…re is no internet.

AnirudhBhat added type: bug A confirmed bug. feature: point of sale POS project labels Sep 10, 2024

AnirudhBhat added this to the 20.4 milestone Sep 10, 2024

AnirudhBhat added 2 commits September 10, 2024 09:44

Add a dedicated string resource for POS

76396fb

Use the new POS string resource

364c257

AnirudhBhat requested a review from samiuelson September 10, 2024 04:23

AnirudhBhat marked this pull request as ready for review September 10, 2024 04:23

AnirudhBhat mentioned this pull request Sep 10, 2024

[Woo POS] Improve Card Reader Payment Flow Error Handling #12543

Merged

4 tasks

samiuelson self-assigned this Sep 12, 2024

samiuelson reviewed Sep 12, 2024

View reviewed changes

backwardstruck and others added 3 commits September 12, 2024 12:26

Merge branch 'trunk' into issue/12497-pos-internet-issue

8927c2e

Remove new line from the no internet string resource.

b72a65f

Use data class to represent toast message.

f49db88

AnirudhBhat requested a review from samiuelson September 13, 2024 05:38

samiuelson approved these changes Sep 13, 2024

View reviewed changes

AnirudhBhat added the unit-tests-exemption label Sep 13, 2024

Merge branch 'trunk' into issue/12497-pos-internet-issue

65f8eeb

AnirudhBhat enabled auto-merge September 13, 2024 07:30

AnirudhBhat merged commit 618cb95 into trunk Sep 13, 2024
14 checks passed

AnirudhBhat deleted the issue/12497-pos-internet-issue branch September 13, 2024 07:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Woo POS] Cannot reconnect to Reader after turning internet off and on #12569

[Woo POS] Cannot reconnect to Reader after turning internet off and on #12569

AnirudhBhat commented Sep 10, 2024 •

edited

Loading

dangermattic commented Sep 10, 2024 •

edited

Loading

wpmobilebot commented Sep 10, 2024 •

edited

Loading

wpmobilebot commented Sep 10, 2024 •

edited

Loading

codecov-commenter commented Sep 10, 2024 •

edited

Loading

samiuelson left a comment

samiuelson Sep 12, 2024

AnirudhBhat Sep 13, 2024

samiuelson Sep 12, 2024

AnirudhBhat Sep 13, 2024

samiuelson left a comment

[Woo POS] Cannot reconnect to Reader after turning internet off and on #12569

[Woo POS] Cannot reconnect to Reader after turning internet off and on #12569

Conversation

AnirudhBhat commented Sep 10, 2024 • edited Loading

Description

Steps to reproduce

Testing information

Images/gif

Reviewer (or Author, in the case of optional code reviews):

dangermattic commented Sep 10, 2024 • edited Loading

wpmobilebot commented Sep 10, 2024 • edited Loading

wpmobilebot commented Sep 10, 2024 • edited Loading

codecov-commenter commented Sep 10, 2024 • edited Loading

Codecov Report

samiuelson left a comment

Choose a reason for hiding this comment

samiuelson Sep 12, 2024

Choose a reason for hiding this comment

AnirudhBhat Sep 13, 2024

Choose a reason for hiding this comment

samiuelson Sep 12, 2024

Choose a reason for hiding this comment

AnirudhBhat Sep 13, 2024

Choose a reason for hiding this comment

samiuelson left a comment

Choose a reason for hiding this comment

AnirudhBhat commented Sep 10, 2024 •

edited

Loading

dangermattic commented Sep 10, 2024 •

edited

Loading

wpmobilebot commented Sep 10, 2024 •

edited

Loading

wpmobilebot commented Sep 10, 2024 •

edited

Loading

codecov-commenter commented Sep 10, 2024 •

edited

Loading